Coding Challenge Submission - #398
Open
okimdaniel wants to merge 12 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements a Google carousel parser and outputs a JSON list in the expected structure.
Stack
Setup
.ruby-versionfor asdf/mise/rvmbundle installbin/extractto get the output of the provided html file. You can also pass in a specific html file to parse, for example:bin/extract spec/fixtures/pages/mandalorian-cast.htmlUse
bundle exec rspecto run testsThe approach
I started with using nokolexbor to parse the provided html file using html structure (
data-attridandrole="list"/role="listitem") rather than relying on class names for selecting elements. I quickly realized that the thumbnails are set at runtime with Javascript in a<script>tag or lazy loaded from a JSON object once the carousel is scrolled into view.I chose to extract the images from the JS with regex instead of having a browser or something like QuickJS interpret and execute the code in the script tags. This includes logic to extract lazy loaded thumbnails from a
google.ldimap as well. I felt that this was an acceptable tradeoff given the requirements and it wouldn't be difficult to update the code to an approach that executes Javascript if needed.Once I had an initial implementation, I added rspec tests and started testing with other types of carousels. My initial implementation was overfit on artwork so I changed it to parse for the largest list of single elements containing an image, caption, and link combination. This allowed me to add support for non-artwork types and I decided to remove the
artworkskey from the expected response.For general testing strategy, I used a "golden" set of input html files, extracted the JSON array, and compared it against a known good set of resulting JSON files. It reads all files in
spec/fixtures/pagesandspec/fixtures/expectedwhich makes it easy to add/remove tests over time. I've also added unit tests checking parsing logic/guarantees.Notes & Tradeoffs
nullwhen I can't find a thumbnail image instead of returning the default placeholder GIF.artworkskey from the expected results structure and returning the bare JSON array to add support for non-artwork carousel types. I can update it to include theartworkskey if it's a requirement.